Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clip point in PointPrimitiveCollection properly #8542

Merged
merged 4 commits into from
Jan 16, 2020

Conversation

Samulus
Copy link
Contributor

@Samulus Samulus commented Jan 15, 2020

Fixes #7557 , still unsure if it has any unattended side effects! Posting what I have now.

@cesium-concierge
Copy link

Thanks for the pull request @Samulus!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@@ -135,7 +135,7 @@ void main()
float nearSq = distanceDisplayConditionAndDisableDepth.x;
float farSq = distanceDisplayConditionAndDisableDepth.y;
if (lengthSq < nearSq || lengthSq > farSq) {
positionEC.xyz = vec3(0.0);
positionEC.xyz = vec3(1.0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So instead of putting the position at the center of the earth we're putting it at 1,1,1 and that fixes it? Seems kind of odd to me and maybe masking a bigger issue?

Random idea, is the problem here that positionEC is actually a vec4 and not a vec3 and something like:

positionEC.xyz = vec3(0.0);
positionEC.w = 1.0;

Is the more correct fix?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking is that positionEC.xyz = vec3(1.0); puts the point behind the camera since negative Z is forward in eye space. It should get clipped afterwards in the pipeline.

Probably a clearer fix is to do positionEC.xyz = vec3(0.0, 0.0, 1.0); and have an inline comment explaining why.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the range for eye coordinates again? Is 1.0 on the near plane? Maybe use 2.0?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I was reading poistionEC as earth-centric, now eye coordinates. (forgetting we use WC for world coordinates)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still a little strange that positionEC.xyz = vec3(0.0); doesn't work. That should be putting the point behind the near plane. I just noticed that the bug only happens when log depth is true. And it doesn't matter if I change the log-depth near plane default from 0.1 to 1.0.

Still, positionEC.xyz = vec3(0.0, 0.0, 1.0); should be safe because it is 1 meter behind the camera, and either 1.1 or 2.0 meters behind the near plane (for log-depth and multi-frustum respectively).

@Samulus
Copy link
Contributor Author

Samulus commented Jan 16, 2020

I've verified that the clipping in the Display Distance Conditions Sandcastle example doesn't have any regressions caused by this branch. Are there any other demos I should test explicitly?

@lilleyse
Copy link
Contributor

Looks like that's the only sandcastle that involves points and distance display conditions. Can confirm that both that sandcastle and the one in #7557 are fixed by this PR.

I'm going to open a separate issue for standardizing how we hide/clip geometry in shaders. Looking around the code there are several different methods.. all slightly different.

@lilleyse
Copy link
Contributor

Opened that issue here: #8547.

No need to work on it right now, I just wanted to get all my thoughts together.

@lilleyse
Copy link
Contributor

Thanks @Samulus!

@lilleyse lilleyse merged commit ac7e9ee into master Jan 16, 2020
@lilleyse lilleyse deleted the 7557/point_in_middle_of_screen branch January 16, 2020 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DistanceDisplayCondition point in the middle of the Viewer
5 participants